[b/331681978] Add Short URL matching to KaggleFileResolver #1375
Conversation
| self.assertEqual([1, 1], layer(test_inputs).shape) | ||
| # Delete the files that were created in KaggleJwtHandler's do_POST method |
There was a problem hiding this comment.
This was needed or else os.makedirs and os.symlink would fail when the new test tried to recreate the same files. (Alternative solution was to use a different model URL for the new test, but I thought it was cleaner to do the proper cleanup after each test.)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -4,18 +4,19 @@ | |||
|
|
|||
| from tensorflow_hub import resolver | |||
|
|
|||
| url_pattern = re.compile(r"https?://([a-z]+\.)?kaggle.com/models/(?P<owner>[^\\/]+)/(?P<model>[^\\/]+)/frameworks/(?P<framework>[^\\/]+)/variations/(?P<variation>[^\\/]+)/versions/(?P<version>[0-9]+)$") | |||
| short_url_pattern = re.compile(r"https?://([a-z]+\.)?kaggle.com/models/(?P<owner>[^\\/]+)/(?P<model>[^\\/]+)/(?P<framework>[^\\/]+)/(?P<variation>[^\\/]+)/(?P<version>[0-9]+)$") | |||
| long_url_pattern = re.compile(r"https?://([a-z]+\.)?kaggle.com/models/(?P<owner>[^\\/]+)/(?P<model>[^\\/]+)/frameworks/(?P<framework>[^\\/]+)/variations/(?P<variation>[^\\/]+)/versions/(?P<version>[0-9]+)$") | |||
There was a problem hiding this comment.
I was considering if we should load these via an ENV var that we can mutate, but I suppose it is really rare that we mutate this, and it might be fine if only new images can support changes like this going forward.
| @@ -4,18 +4,19 @@ | |||
|
|
|||
| from tensorflow_hub import resolver | |||
|
|
|||
| url_pattern = re.compile(r"https?://([a-z]+\.)?kaggle.com/models/(?P<owner>[^\\/]+)/(?P<model>[^\\/]+)/frameworks/(?P<framework>[^\\/]+)/variations/(?P<variation>[^\\/]+)/versions/(?P<version>[0-9]+)$") | |||
| short_url_pattern = re.compile(r"https?://([a-z]+\.)?kaggle.com/models/(?P<owner>[^\\/]+)/(?P<model>[^\\/]+)/(?P<framework>[^\\/]+)/(?P<variation>[^\\/]+)/(?P<version>[0-9]+)$") | |||
| long_url_pattern = re.compile(r"https?://([a-z]+\.)?kaggle.com/models/(?P<owner>[^\\/]+)/(?P<model>[^\\/]+)/frameworks/(?P<framework>[^\\/]+)/variations/(?P<variation>[^\\/]+)/versions/(?P<version>[0-9]+)$") | |||
There was a problem hiding this comment.
Here's a unified version of the two regex's if we wanted to still only need one:
https?://([a-z]+\.)?kaggle.com/models/(?P<owner>[^\\/]+)/(?P<model>[^\\/]+)(/frameworks)?/(?P<framework>[^\\/]+)(/variations)?/(?P<variation>[^\\/]+)(/versions)?/(?P<version>[0-9]+)$
https://regex101.com/r/JbWJNI/1
All I did was making the "extra" parts of the path optional, though I suppose this does make each part independently optional.
There was a problem hiding this comment.
Would you mind if we keep short_url_pattern and long_url_pattern? I lean towards that because it feels more readable and avoids the "independently optional parts" issue you mention
This change is required to unblock https://github.com/Kaggle/kaggleazure/pull/28591, which updates the Kernel MT to only use short URLs.
Testing
Added unit test and ran ./test